gc: re-derive shadow-stack slots across visitor calls, and guard a walk in debug - #917
Conversation
…lk in debug Both shadow-stack walkers held a borrow of the thread-local `Vec` across the caller-supplied visitor. `pin_root` pushes onto that same `Vec`, so a visitor that re-entered it could reallocate the buffer and leave the walk reading and writing through the old allocation. Re-derive the slot address and re-read the length after every visitor call, so the walk no longer depends on the storage staying put. This narrows the hazard rather than removing it: the `&mut PyObjectRef` handed to a visitor still points into the buffer for the duration of that call, which is intrinsic to walking a growable container. `shadowstack.py:344-349` sizes the root stack once at `root_stack_depth` (`:281`) and `incr_stack` (`:80-84`) is a bare pointer bump, so upstream's push never moves the storage; the growable `Vec` here is what makes a re-entrant push a memory-safety question at all. `walk_shadow_stack_area` constructed no `ShadowStackAccess`, so it had no re-entrancy check in any build — and it is the walker that actually runs, since `walk_shadow_stack`'s only non-test caller is never registered. Before the `UnsafeCell` change it read through `RefCell::as_ptr()`, which bypasses the borrow flag just as completely, so this walker has never been checked. Add a debug-only per-thread walk flag, asserted in `pin_root`, `shadow_stack_set` and `RootScope::drop`, covering both walkers. It is a separate flag from `SHADOW_STACK_ACCESS_DEPTH` because the exclusive access no longer spans the visitor call, so the depth counter alone would see nothing. `RootScope` held a bare `usize` and was therefore auto-`Send`; dropping one on another thread truncates that thread's stack to an unrelated save point. Add a `PhantomData<*const ()>`. The hot read path (`shadow_stack_get`, `shadow_stack_len`, `shadow_stack_copy_range`) is unchanged. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe change adds debug-time protection against shadow-stack mutation during root walks, makes ChangesShadow-stack walk safety
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant RootScope
participant ShadowStackWalk
participant Visitor
participant ShadowStack
RootScope->>ShadowStackWalk: start root walk
ShadowStackWalk->>ShadowStack: reacquire current slot
ShadowStackWalk->>Visitor: visit root
Visitor->>ShadowStack: mutate or reallocate stack
ShadowStackWalk->>ShadowStack: recompute next slot address
ShadowStackWalk->>Visitor: visit next root
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af656b0416
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let cell = cell as *const UnsafeCell<Vec<PyObjectRef>>; | ||
| #[cfg(debug_assertions)] | ||
| SHADOW_STACK_WALK_IN_PROGRESS.with(|in_progress| { | ||
| let _walk = ShadowStackWalk::new(in_progress); |
There was a problem hiding this comment.
In debug builds, this replaces the exclusive access guard that previously spanned the visitor with a flag checked only by mutating APIs. A safe visitor can now call shadow_stack_get(0) and subsequently write through its slot; the getter creates a shared reference to the same element while the visitor's mutable reference remains live, violating Rust's aliasing rules without triggering any assertion. Make shared accessors consult the walk flag as well, or otherwise avoid exposing an aliased &mut slot.
Useful? React with 👍 / 👎.
| #[cfg(debug_assertions)] | ||
| assert_shadow_stack_not_walking(); |
There was a problem hiding this comment.
When a debug-build visitor follows the documented pattern let _roots = push_roots(); pin_root(...), pin_root first panics because a walk is active, then unwinding drops _roots and this new assertion panics a second time, aborting the process instead of producing the intended catchable diagnostic. Reject push_roots/RootScope::new before a guard is constructed, or make cleanup during an existing panic non-panicking.
Useful? React with 👍 / 👎.
`walk_stack_root` (`shadowstack.py:43-46`) takes `start` and `addr` as arguments and runs `while addr != start`, so the interval a root walk covers is fixed when the walk begins and a root pushed mid-walk is not part of it. Re-reading the length each iteration extended the walk over roots the visitor itself pinned — and pinning during a walk is what the walk guard added alongside declares illegal, so the divergence served nothing. The slot address is still re-derived every iteration: a re-entrant push can reallocate the buffer, and a cached cursor would be left in the old allocation. Only pushes can occur during a walk, so the entry length stays in bounds. This sharpens the release test rather than weakening it. Reading `0x22` correctly now happens on the iteration after the visitor forced the reallocation, so that assertion is the use-after-free discriminator. Assisted-by: Claude
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit da7e32f). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
|
Second commit pushed: the parity correction, plus the full gate re-run against it on the new base. The review changed the design, not just the wordingThe first commit re-read The Codex parity review called it a regression, and checking the source settles The flaw in my argument was self-inflicted. The case it protected — pinning
It also sharpens the release test instead of weakening it. Verification, all of it re-run against
|
gc_roots debug (incl. the should_panic walk-guard test) |
7 passed |
| release-only reallocation test | 1 passed — confirmed it runs, not filtered out |
pyre-object full |
293 passed |
cargo check -p pyrex --features dynasm (the !Send blast radius) |
Finished, no errors |
gc_stress |
23 passed, 0 failed |
The earlier gate was discarded rather than reported: it was building while the
parity edit landed, so its result was neither pre-edit nor post-edit. This one
ran start to finish on a clean tree at the committed sha.
Note on this PR's own review coverage
The codex-review check going green does not mean a parity report exists — the
job is named "Queue Codex parity review" and only the queueing succeeded. #909,
#910 and #911 were all green with no report at all after the runner's codex
token expired, and the last two merged that way. The review acted on above came
from running it locally against the real merge-base; the skill's default
upstream/main was 19 commits stale and would have diffed other people's merged
work into this patch.
— commented by Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/da7e32ff80bc43b9b66710815df7b6df9a468033/pyre-object/src/gc_roots.rs#L486
In release builds, a safe walk_shadow_stack visitor can call pin_root enough times to grow the backing Vec, as the new release-only test does. That reallocation invalidates the &mut PyObjectRef passed here while the callback is still executing; re-deriving the next slot only after the callback returns is too late, so this safe API can invoke undefined behavior. Preserve the upstream stable shadow-stack storage shape, or prohibit mutation during walks in release builds rather than handing out references into movable storage.
AGENTS.md reference: AGENTS.md:L194-L196
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Follow-up to the Codex review on #909. Acts on the finding, but not with the
attribution the finding gave it.
What the review said, and what is actually true
The mechanism is right.
walk_shadow_stackheldstack.iter_mut()acrossvisitor(slot), andpin_root'sVec::pushcan reallocate underneath it.The attribution is not.
walk_shadow_stackis dead outside tests — its onlynon-test caller,
pyre_object_root_walker, is never registered. The walker thatactually runs is
walk_shadow_stack_area, and before #909 that one read throughRefCell::as_ptr(), which bypasses the borrow flag exactly as completely as anUnsafeCelldoes. So on the live path #909 removed no check; there was none.What the investigation did turn up is worse than the reported regression: the
live walker had no re-entrancy check in any build, before or after.
Reachability
Not reachable today. Following both visitor chains to their leaves — including
the
DictStrategy::walk_gc_refsdyn call, whose implementors are a closed setof raw-storage-plus-visitor bodies, and the collector-side closures
(
drag_out_root,enumerate_root_walker_values) — finds no call back into anygc_rootsAPI.majit-gccannot even namepyre_object::gc_roots; its onlydependencies are
indexmap,majit-irandlibc. Three independent attemptsto refute that — from the collector-callback, test-tooling and
indirect-mutation (finalizer / nested collection /
RootScope::drop) angles —each failed to produce a chain.
#868landing in the base does not change this:execute_finalizer_triggersfires once at the end of a major step, after the root walk, and only notifies
the death deques — "pyre has explicit death deques but no collector-run
execute_finalizersphase". No Python runs inside a walk.So this is defence in depth, and the change is scoped to match.
Change
visitor call, so the walk no longer depends on the storage staying put.
pin_root,shadow_stack_setandRootScope::drop, covering both walkers. It has to bea separate flag from
SHADOW_STACK_ACCESS_DEPTH: once the exclusive accessno longer spans the visitor call, the depth counter would see nothing.
RootScopeheld a bareusizeand was therefore auto-Send; dropping one onanother thread truncates that thread's stack to an unrelated save point. Now
!SendviaPhantomData<*const ()>.The hot read path —
shadow_stack_get,shadow_stack_len,shadow_stack_copy_range— is untouched, so none of #909's measured 9.8% is atstake.
What this does NOT fix
It narrows the hazard; it does not remove it. The
&mut PyObjectRefhanded to avisitor still points into the buffer for the duration of that call, so a visitor
that pins and then writes through its own slot is still unsound. That is
intrinsic to walking a growable container.
Passing the value by copy and writing it back would close it, and I rejected
that: the collector threads the real slot address into its diagnostics
(
copy_nursery_object(..., slot_addr)), and "GC BUG: traced slot containsnursery poison at slot_addr=…" would then point at a stack temporary.
Upstream does not have the problem at all.
shadowstack.py:344-349sizes theroot stack once at
root_stack_depth(:281) andincr_stack(:80-84) is abare pointer bump with no bounds test — a push never moves the storage.
Converging on that fixed-capacity shape is the real close; it carries a
per-thread memory trade-off (163840 entries ≈ 1.3 MB) that deserves its own
change and its own measurement.
Parity correction after review
The first commit re-read
len()each iteration, which extended a walk overroots the visitor itself pinned. The Codex parity review flagged that as a
regression and it was right:
walk_stack_root(shadowstack.py:43-46) takesstartandaddras arguments and runswhile addr != start, so the intervalis fixed when the walk begins.
My justification for diverging had been that a root pinned mid-walk would
otherwise go unforwarded and dangle. That argument does not survive contact
with the rest of this PR: pinning during a walk is exactly what the walk guard
here declares illegal, so the divergence protected a case this change makes a
debug panic. The second commit fixes the interval at entry and keeps the
per-iteration slot re-derivation, which is what actually defends against a
reallocation.
It also sharpens the release test. Reading
0x22correctly now happens on theiteration after the visitor forced the reallocation, so that assertion is the
use-after-free discriminator on its own; previously the property was buried in
a length check.
Verification
gc_rootsdebug tests 7 passed (including a newshould_panictest that thewalk guard actually fires), release-only reallocation test 1 passed — confirmed
it genuinely runs rather than being compiled out —
pyre-object283 passed,cargo check -p pyrex --features dynasmclean (this is the blast radius of the!Sendchange),gc_stress23 passed / 0 failed.Those counts are from the first commit.
origin/mainadvanced to #868mid-session and the branch was rebased onto it (the
gc_roots.rsblob isbyte-identical across the rebase), and the parity correction above landed
afterwards, so the whole gate is re-running on the new base against the final
commit. I will post the result.